Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new merkle tree implementation that uses RocksDb for storage, adds support for two modes (archive and nomt), updates compiler flags and CI configuration, and performs a namespace rename from “jam” to “morum”. Key changes include dependency and build configuration updates in vcpkg, extensive namespace renaming across source/test files, and the complete implementation of the merkle tree functionality.
Reviewed Changes
Copilot reviewed 147 out of 148 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vcpkg.json | Updated package name and dependencies; dependency renames and removals performed. |
| vcpkg-overlay/* | Updated portfile and package names to align with new namespace. |
| test/* and src/* | Renamed namespaces (from jam to morum) and integrated new merkle tree implementation changes. |
| return qtils::FixedByteSpanMut<DISK_PAGE_SIZE>{ | ||
| reinterpret_cast<uint8_t *>(this), DISK_PAGE_SIZE}; |
There was a problem hiding this comment.
| return qtils::FixedByteSpanMut<DISK_PAGE_SIZE>{ | |
| reinterpret_cast<uint8_t *>(this), DISK_PAGE_SIZE}; | |
| return {reinterpret_cast<uint8_t *>(this), DISK_PAGE_SIZE}; |
There was a problem hiding this comment.
It doesn't compile because this constructor is explicit.
| std::copy_n(value.begin(), value.size(), value_hash.begin()); | ||
| std::fill_n(value_hash.begin() + value.size(), 32 - value.size(), 0); | ||
| } else { | ||
| bytes[0] = 0x3; |
There was a problem hiding this comment.
| bytes[0] = 0x3; | |
| bytes[0] = 0b00000011; |
There was a problem hiding this comment.
I think it's easy enough to image 3 in binary system, I'd like to mostly stick to hex, because it's more compact.
There was a problem hiding this comment.
Please, refactor it after merge of master.
Move tests into unit directory.
Please, use google-test framework where it possible.
benchmark/merkle_tree/set_get.cpp
Outdated
| int main(int argc, char **argv) { | ||
| char arg0_default[] = "benchmark"; | ||
| char *args_default = arg0_default; | ||
| if (!argv) { |
There was a problem hiding this comment.
if (argc == 1)
argv это массив указателей и он не может быть равен nullptr. А argv[0] это путь к исполняемому файлу
There was a problem hiding this comment.
Yeah, it was meant to be !*argv. argc can equal zero.
| { | ||
| ZoneNamedN(setter_zone, "set", true); | ||
| for (auto &[k, v] : insertions) { | ||
| tree->set(k, qtils::ByteVec{v}).value(); |
There was a problem hiding this comment.
Возможно тут стоит вставить ассерт на наличие результата
There was a problem hiding this comment.
It will throw an exception and kill the benchmark, which in this place works fine in my opinion.
| ZoneNamedN(setter_zone, "initial insertions", true); | ||
|
|
||
| for (int i = 0; i < INSERTION_NUM; i++) { | ||
| tree->set(random_hash(), qtils::ByteVec{random_vector()}).value(); |
There was a problem hiding this comment.
Возможно тут стоит вставить ассерт на наличие результата
There was a problem hiding this comment.
It will throw an exception and kill the benchmark, which in this place works fine in my opinion.
| ZoneNamedN(getter_zone, "get", true); | ||
|
|
||
| for (auto &[k, v] : insertions) { | ||
| tree->get(k).value(); |
There was a problem hiding this comment.
Возможно тут стоит вставить ассерт на наличие результата
There was a problem hiding this comment.
It will throw an exception and kill the benchmark, which in this place works fine in my opinion.
| set(CMAKE_CXX_STANDARD 23) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) |
There was a problem hiding this comment.
Don't off extensions, please. We are using "statement expressions" in some places.
3 participants
Uh oh!
There was an error while loading. Please reload this page.